Skip to content

Conversation

@ulgens
Copy link
Member

@ulgens ulgens commented Dec 17, 2025

Coming from #356 (comment)

I checked the average runtimes from https://github.com/django-commons/membership/actions/metrics/performance and tried to set meaningful but not too strict limits. Please let me know if you think the values should be updated.

Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plan and Apply are very similar. I think we should use the same timeout. I don't think it should be more than 10, 5 may be a bit too short, but I could agree to it. I put suggestions to settle on 10 as more conservative, but I'll likely switch to your opinion @ulgens if you have a strong preference.

ulgens added a commit to ulgens/membership that referenced this pull request Dec 17, 2025
django-commons#359 (comment)

Co-authored-by: Tim Schilling <schilling711@gmail.com>
ulgens added a commit to ulgens/membership that referenced this pull request Dec 17, 2025
django-commons#359 (comment)

Co-authored-by: Tim Schilling <schilling711@gmail.com>
@ulgens
Copy link
Member Author

ulgens commented Dec 17, 2025

@tim-schilling I committed both of your suggestions. We can "squash merge" the PR at the end.

I don't have a strong opinion on the values; I'm actually a bit lost about them.

  • add_member takes 11 seconds and member_verification takes 6 seconds on average. If these tasks take over a minute, it means they take 6x - 10x longer than usual, which could mean there is an issue, but I am not sure if we want to cut a task at a minute. Even though it's way over the average time, it's not long enough to have any negative effects on the CI setup.
  • plan and apply are similar in their nature but
    • plan takes 6m5s in average. 10 minutes means ~65% longer than usual.
    • apply takes 2m49s in average. 10 minute means ~205% longer than usual.

and I'm not sure if any of these analyses are actually important 😬 I tend to set a limit with a logic that "if something takes a certain percentage longer than average, it should be timed out" but even if we use a global value like 15m, I'm not sure there would be any harm.

@tim-schilling
Copy link
Member

I may need to rebase/squash these so my name is associated with them. There's a bug in the plan action 🫠

@tim-schilling
Copy link
Member

Nope, that wasn't it. Sorry @ulgens if this gets a little noisy.

Co-authored-by: Tim Schilling <schilling711@gmail.com>
@cunla
Copy link
Member

cunla commented Dec 19, 2025

I think this won't run plan since it does not have access to the TF github token secret.

@tim-schilling
Copy link
Member

@cunla any ideas on why it doesn't?

@tim-schilling
Copy link
Member

Ah, because it's a fork, not the actual branch. Hmmm

@cunla
Copy link
Member

cunla commented Dec 19, 2025

Ah, because it's a fork, not the actual branch. Hmmm

I think we want to keep it that way, otherwise forks will have access to the github_token.
that's the diff between pull_request_target and pull_request trigger

@tim-schilling
Copy link
Member

Replaced by #364. Thank you @ulgens!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants